Was thinking about the useMemo's dependencies list, and I'm thinking that we could simplify it even further if we only wanted to match this logic: given that the dependencies are object refs, this is pure ref equality, so we could technically simplify edited to template !== initialTemplate.

The current memoization logic is almost that, just that when the refs are equal, it will do the JSON comparison. But because we use setTemplate on any edit, I think it is effectively exactly the same. So I'm pretty sure the simplification would be equivalent.

What do you think?

(something something "react compiler" something something "dependencies list are too easy to make mistakes with" something something)

I tried that initially, but there's a couple problems with relying on object equality:

1. If you make an edit, then undo that edit, edited will remain true.
2. If you click "Update", edited will remain true. I think this is happening because of how the <ObjectEditor> serializes/deserializes the value. When "Update" is fired, the <ObjectEditor> component will be re-rendered with the new value, which gets serialized here:

   argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 25 in 6cac182

   const [text, setText] = useState<string>(stringify(value, lang));

   
   Then, the onChange handler is fired (which is bound to setTemplate()), which parses it back to an object:

   argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 132 in 6cac182

   onChange(parse(v));

   
   Even though the object is going to have the same properties, it's still considered distinct.

Yes, that's correct; this would've been the previous behavior (prior to this bug) as well if I'm not mistaken.
So while the JSON equality is more accurate than the previous behavior, it does have a performance trade-off, which is perhaps not worthwhile solely for the sake of UI button accuracy.

just that when the refs are equal, it will do the JSON comparison. But because we use setTemplate on any edit,

I did make a mistake in my previous statement^ though, "equal" -> "unequal". Meaning the JSON comparison will pretty much happen on every change, so the useMemo is effectively not used as the refs are never equal.

Since this is an input box that is latency sensitive, I'm thinking that simplifying it to the previous behavior, even if it's not entirely accurate, would be worthwhile compared to a JSON comparison on each change, as the useMemo is effectively ignored.

I checked, and on the main branch, the stringify() function is called 4 times for every change made in <ObjectEditor>:

1. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 25 in 5a2fa4e

   const [text, setText] = useState<string>(stringify(value, lang));

2. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 29 in 5a2fa4e

   useEffect(() => setText(stringify(value, lang)), [value]);

3. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 29 in 5a2fa4e

   useEffect(() => setText(stringify(value, lang)), [value]);

   (again)

4. argo-workflows/ui/src/app/shared/components/object-editor.tsx

   Line 38 in 5a2fa4e

   const editorText = stringify(parse(editor.current.editor.getValue()), lang);

It'd surprise me if adding a couple more calls would have a significant performance impact, but you're right that making performance worse might not be worth fixing this bug. I pushed 24c0206 to delete the isEqual() logic and go back to direct object comparisons.

To fix this without affecting performance, I think we need to refactor <ObjectEditor> to only work with strings, not parsed objects. Parsing will have to be lifted up to the details component (<ClusterWorkflowTemplateDetails> in this case), which can then easily compare strings to see what's changed. That's not hard, but it does involve quite a lot of changes, so it's probably best done in a separate PR.

Yea that first one could be made into a callback function instead very simply. The last one has that comment around it regarding whitespace

I've optimized the rest of the UI by code-splitting this component before in #12150 so this component is a bit of a hassle 😕

but you're right that making performance worse might not be worth fixing this bug.

yea the buttons working even though there has been no semantic change is pretty tiny and fairly harmless. I'd argue it's maybe even worth further simplifying if possible just to remove more complex code.

Otherwise if we wanted to keep a bunch of features while being latency sensitive, a separate thread / web worker might be the way to go to keep the UI thread light.

Here's a POC of what I was suggesting: main...MasonM:argo-workflows:poc-fix-13453

The <ObjectEditorPlain> component is a copy of <ObjectEditor> for POC purposes. The parsing and language handling code has been lifted up to the details component, where it's encapsulated in the useEditableObject() hook.

This completely eliminates all the unnecessary calls to stringify() when there's a change, and also fixes the bugs with the "Update" button. It does call parse() once on a change, but that's an improvement from the main branch, where it's called twice.

If that looks good to you, I can clean that up and enter a PR.

Since this is JSDoc and not GoDoc, we can simplify this to be less repetitive / verbose. There's also a typo "that be edited"

We should also mention the ref comparison same as we did the JSON comparison in the isEqual function before. And can one-line it

It also could make sense to rename this function/file use-editable-object to match object-editor.

Maybe even makes sense to have as part of the ObjectEditor component? 🤔 EDIT: I wrote that confusingly; I meant as an exported function in the same file, see below comment

The other hooks that have documentation follow the <x> is a React hook format:

I updated the comment in cf9a3f3 to match, and renamed it to use-editable-object.

I don't see how this could be part of <ObjectEditor> because the edited variable is needed by the details components.

The other hooks that have documentation follow the <x> is a React hook format:

Ah that works for consistency then I suppose. Some of the original ones written were almost certainly bringing GoDoc conventions into JSDoc (there's a handful of non-JS idioms in the UI here and there).

I don't see how this could be part of <ObjectEditor> because the edited variable is needed by the details components.

I meant as another exported function inside the same file, not directly integrated into the component. Sorry I can see how my wording above could be confusing.

As in, there are some conventions in the ecosystem of import {useComponent, Component} that we could follow. Although the usage is a tad different here with regard to the tracked state and props (they are not quite matching)

@MasonM any thoughts on the above? Otherwise this LGTM and I can merge

I don't think it makes sense to couple this to <ObjectEditor>, because that will make it more difficult to move forward with the approach I posted above in #13593 (comment). That POC fully decouples <ObjectEditorPlain> from all serialization/deseralization and moves that logic into useEditableComponent(), which significantly improves performance and is a bit cleaner (IMO). Of course, that's a fairly significant change, and probably should come after this PR, but I think we'll want to do this eventually.

But if you feel strongly, I can go ahead and make that change.

I don't feel strongly.
I didn't read that change too closely yet so from a quick look I actually thought putting the imports/files together might help move in that direction, but I might've been mistaken. Since the performance change may make it even more likely that any consumer of the component would want to also use the hook (and it is currently used like that) and so combining the imports makes it more obvious how to use.

useEditableComponent()

I guess your intent with this naming is that the hook could in the future be extended to other editable components as well? As in that it's not specific to ObjectEditor per se

I guess your intent with this naming is that the hook could in the future be extended to other editable components as well?

Honestly, that was a typo. I meant useEditableObject(). It could theoretically be extended to other editable components, but I can't think of anything that would benefit.

Oh ok 😅 Then I think it would make sense to combine the imports / exports so that it's more obvious the hook is supposed to be used together with the component: import {ObjectEditor, useEditableObject}

Technically doesn't have to be in the same file, could have an import/export stub instead